Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

Improve support for URL query parameters #96

Closed
wants to merge 3 commits into from
Closed

Improve support for URL query parameters #96

wants to merge 3 commits into from

Conversation

mehaase
Copy link

@mehaase mehaase commented Mar 19, 2015

Route.dart's support for query parameters, e.g. ?foo=bar&baz=bat is abysmal.

I spent almost 3 full days trying to hack around all of its idiosyncracies and finally gave up in frustration. It turns out to be much easier just to fix route.dart.

Caveats

  1. I've focused on HTML5 mode, because I don't know enough about routing with the URL fragment to do it right.
  2. I started to write a unit test for the click handler but it complains that MockWindow doesn't have an onPopState member, and it seems like a lot of work to properly mock onPopState. There are no existing tests for HTML5 routing that I can mimic — the tests are exclusively for fragment routing.

1. Query parameters are observed on initial page load. (They were
   previously ignored.)
2. Query parameters are preserved when clicking on an <a> element.
   (They were previously discarded.)
3. Hash fragment is preserved when clicking on an <a> element in
   HTML5 mode. (No change to behavior when using URL fragment for
   routing.)
4. Browser's backward/forward preserve query parameters (They were
   previously discarded.)
@mehaase
Copy link
Author

mehaase commented May 6, 2015

@pavelgj I see you commenting on other PRs today... can I convince you to look at mine?

} else if (anchor.hash == '') {
_router.gotoUrl('${anchor.pathname}${anchor.search}');
} else {
Element el = document.querySelector(anchor.hash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make sense... why are you querying by the value of the hash?

@pavelgj
Copy link
Contributor

pavelgj commented May 27, 2015

Merged via 7cd7257
Did not include hash scrolling on pushState -- too opinionated. You can always use a custom WindowClickHandler if that functionality is required.

@pavelgj pavelgj closed this May 27, 2015
@MikeMitterer
Copy link

How would you define a custom WindowClickHandler? You need a linkMatcher, router asf... https://github.com/angular/route.dart/blob/master/lib/click_handler.dart
Why is this opinionated? It the router finds a route - _router.gotoUrl(...) if it doesn't find a matching route - use the default behavior: Scroll to anchor...

@pavelgj
Copy link
Contributor

pavelgj commented Jul 7, 2015

if it doesn't find a matching route - use the default behavior: Scroll to anchor

When you put is this way, it makes sense. However it's not what your code does:

   Element el = document.querySelector(anchor.hash);
   if (el != null) {
     Rectangle r = el.getBoundingClientRect();
     _window.scroll(0, r.top.floor());
   }

AFAIK, default browser behavior is something more like

   Element el = document.querySelector('a[name=${anchor.hash}]');

But in this case sanitizing that hash to fit into CSS selector would be a pain...
If you're feeling strongly about this feature, please send a separate PR, we can discuss all the corner cases there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants